Fix several accidentally quadratic functions#800
Fix several accidentally quadratic functions#800jonjohnsonjr wants to merge 1 commit intospdx:mainfrom
Conversation
Signed-off-by: Jon Johnson <jon.johnson@chainguard.dev>
| ): | ||
| check_types_and_set_values(self, locals()) | ||
|
|
||
| def __hash__(self): |
There was a problem hiding this comment.
this is not a correct hash of the relationship. One might instead return the triple and use it for lookups in the cache
billie-alsup
left a comment
billie-alsup
left a comment
There was a problem hiding this comment.
A few questions and concerns provided inline. Also, it is unclear how spdx_none and spdx_no_assertion are handled in your set. Are they automatically translated to the strings "NONE" and "NOASSERTION" ?
| ): | ||
| check_types_and_set_values(self, locals()) | ||
|
|
||
| def __hash__(self): |
| ) | ||
|
|
||
| package_dicts: List[Dict] = input_doc_dict.get("packages", []) | ||
| existing_relationships_without_comments: List[Relationship] = self.get_all_relationships_without_comments( |
There was a problem hiding this comment.
I'm a bit confused by the removal of this code. The semantics will be different between this and previous call right? This one would include the additional relationships found from the relationships.extend call above. To preserve the previous behavior, it seems that you would instead want to simply retrieve the set identified by parser_field_or_log_error, and then extend relationships with that set. But then you could also update the existing_relationships_without_comments from that new set as well. The question is whether you are purposefully changing the semantics here or not.
|
|
||
| def validate_spdx_id( | ||
| spdx_id: str, document: Document, check_document: bool = False, check_files: bool = False | ||
| spdx_id: str, document: Document, all_spdx_ids: Set[str], check_document: bool = False, check_files: bool = False |
There was a problem hiding this comment.
Should you not also have an files_spdx_ids: Set[str] argument for the check_files case? My SPDX file has over 70K entries. The check_document seems redundant here, since you could simply make all_spdx_ids: Optional[Set[str]] instead and go by whether it was passed as None or not.
| def test_valid_relationship(related_spdx_element): | ||
| relationship = Relationship(DOCUMENT_SPDX_ID, RelationshipType.DESCRIBES, related_spdx_element, comment="comment") | ||
| validation_messages: List[ValidationMessage] = validate_relationship(relationship, "SPDX-2.3", document_fixture()) | ||
| validation_messages: List[ValidationMessage] = validate_relationship(relationship, "SPDX-2.3", document_fixture(), get_all_spdx_ids(document_fixture())) |
There was a problem hiding this comment.
Technically, these are two different documents that have the same content. I would rather see a single document used, as was done for other functions, i.e. save document_fixture() into a local variable, and then use that as an argument to validate_relationship, and to get_all_spdx_ids.
| spdx_element_id=spdx_element_id, related_spdx_element_id=related_spdx_element_id | ||
| ) | ||
| validation_messages: List[ValidationMessage] = validate_relationship(relationship, "SPDX-2.3", document_fixture()) | ||
| validation_messages: List[ValidationMessage] = validate_relationship(relationship, "SPDX-2.3", document_fixture(), get_all_spdx_ids(document_fixture())) |
|
Are there any requirements for backward compatibility of any of the functions whose API has changed? |
3 participants
No description provided.